Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix broken Timeline panel jest test #11827

Merged
merged 2 commits into from
Nov 3, 2023
Merged

Conversation

MidhunSureshR
Copy link
Contributor

@MidhunSureshR MidhunSureshR commented Nov 3, 2023

After matrix-org/matrix-js-sdk#3839, the events are ordered on the basis of origin_server_ts.
We just need to give the call event a larger origin_server_ts compared to the other events so that it appears in the same order as before i.e it appears last.


This change is marked as an internal change (Task), so will not be included in the changelog.

@MidhunSureshR MidhunSureshR requested a review from a team as a code owner November 3, 2023 09:28
@MidhunSureshR MidhunSureshR added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Nov 3, 2023
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite what broke here:, ie. why events are ordered by age / timestamp at all and how that changed it. Seems like something's still failing though. Is there something we could tweak in matrix-org/matrix-js-sdk#3839 as I don't think the intention was to affect event ordering.

@toger5
Copy link
Contributor

toger5 commented Nov 3, 2023

This is very interesting. I had this test failing initially as well. I reran it and then it worked. So there might be ambiguity in the ordering because of the changes from my pr. Is the order of any importance. Otherwise a set instead of the array would make more sense anywyas and we don't need to enforce the order somehow artificially.

@toger5
Copy link
Contributor

toger5 commented Nov 3, 2023

Just to get right what is happening here.
We use the age to order the events? Before the age was just 0 because that is the fallback if the unsigned field is missing. Now we use now - origin server , which is a better approximation for the age then 0. So the code execution order plays a role since sometimes one Ms passes and then they don't have the same age anymore?
The timeline orders by age?

Maybe mocking the age explicitly (in general for the mockEvent fn) would be a cleaner solution then?

@toger5
Copy link
Contributor

toger5 commented Nov 3, 2023

But changing the origin_server_ts is a good fix as well.

@MidhunSureshR
Copy link
Contributor Author

I ran the failing jest test again and it passed so we have a flake

@toger5
Copy link
Contributor

toger5 commented Nov 3, 2023

Is the order of any importance. Otherwise a set instead of the array would make more sense anywyas and we don't need to enforce the order somehow artificially.

what about this?

I ran the failing jest test again and it passed so we have a flake.

I think this is coming from the age now beeing dependent on Date.now(). (is th age used for timeline ordering?)

@MidhunSureshR
Copy link
Contributor Author

I think the order does matter since I expect the events to be ordered in a very specific way when two timelines are merged.
The localTimeStamp property of the event is used for ordering events in this case. I've set them explicitly so it should be okay now.

@toger5
Copy link
Contributor

toger5 commented Nov 3, 2023

I created a draft PR: matrix-org/matrix-js-sdk#3854
How about we merge this one first, and then we can see if the CI is still flaky for matrix-org/matrix-js-sdk#3854?
Then we can then savely merge the fix that for the event age somtimes beeing 0 (which breaks expiration logic for calls)

@MidhunSureshR MidhunSureshR added this pull request to the merge queue Nov 3, 2023
Merged via the queue into develop with commit 9bb5011 Nov 3, 2023
19 checks passed
@MidhunSureshR MidhunSureshR deleted the midhun/fix-broken-jest-test branch November 3, 2023 15:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants